Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix nav to take full width in the docs page #935

Merged
merged 3 commits into from
Sep 9, 2018

Conversation

fiennyangeln
Copy link
Contributor

Fix #911

Motivation

Fix the Nav in the docs page to take the full width so that when scrolled it does not scroll the main container

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

  1. Run the website
  2. Put the devtools to be slightly above the nav (so that we can scroll on it)
  3. Try to scroll slightly outside the nav text
  4. It should scroll the nav instead of the main container

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

No

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 1, 2018
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Sep 1, 2018

Deploy preview for docusaurus-preview ready!

Built with commit ef884ae

https://deploy-preview-935--docusaurus-preview.netlify.com

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll give this a more thorough review soon.

@@ -712,6 +712,11 @@ blockquote {
.mainContainer .wrapper .posts .post {
width: 100%;
}

.docMainContainer {
flex-basis: 920px !important;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid using !importants?

@@ -66,7 +66,7 @@ class DocsLayout extends React.Component {
metadata={metadata}>
<div className="docMainWrapper wrapper">
<DocsSidebar metadata={metadata} />
<Container className="mainContainer">
<Container className="mainContainer docMainContainer">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not directly add the rules within docMainContainer into mainContainer? Is there a need for a new class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because the class is used in other page, like blog so it may break the other page if i modify it directly

@@ -116,7 +116,7 @@ class DocsLayout extends React.Component {
)}
</Container>
{hasOnPageNav && (
<nav className="onPageNav">
<nav className="onPageNav docOnPageNav">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question regarding this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because the class is used in other page, like blog so it may break the other page if i modify it directly

@yangshun
Copy link
Contributor

yangshun commented Sep 2, 2018

There's a difference in the new layout and the current one - The "Getting Started" is no longer flushed with the Docusaurus icon in the navbar.

Old

screen shot 2018-09-02 at 10 18 37 am

New

screen shot 2018-09-02 at 10 12 44 am

@fiennyangeln
Copy link
Contributor Author

@yangshun adjusted based on your review! 😃 please review!

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great after the fix!

@yangshun yangshun merged commit f2dceff into facebook:master Sep 9, 2018
@yangshun
Copy link
Contributor

yangshun commented Sep 9, 2018

Great work @fiennyangeln!

@JoelMarcey
Copy link
Contributor

Thank you @fiennyangeln 😄

@JoelMarcey
Copy link
Contributor

Thanks for the review @yangshun

@SimenB
Copy link
Contributor

SimenB commented Sep 10, 2018

Thanks for fixing it! 🎉

@endiliey
Copy link
Contributor

Great work @fiennyangeln 🎉 Proud as a senior 😭

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants